[lldb] Correct format specifier for sscanf to prevent buffer overflow (NFC) #94783
Conversation
|
@llvm/pr-subscribers-lldb Author: Shivam Gupta (xgupta) ChangesFix #89710 Full diff: https://github.com/llvm/llvm-project/pull/94783.diff 1 Files Affected:
diff --git a/lldb/source/Host/linux/Host.cpp b/lldb/source/Host/linux/Host.cpp
index c6490f2fc9e2f..8a65c46a52ea8 100644
--- a/lldb/source/Host/linux/Host.cpp
+++ b/lldb/source/Host/linux/Host.cpp
@@ -92,7 +92,7 @@ static bool GetStatusInfo(::pid_t Pid, ProcessInstanceInfo &ProcessInfo,
return false;
StatFields stat_fields;
if (sscanf(Rest.data(),
- "%d %s %c %d %d %d %d %d %u %lu %lu %lu %lu %lu %lu %ld %ld",
+ "%d %15s %c %d %d %d %d %d %u %lu %lu %lu %lu %lu %lu %ld %ld",
&stat_fields.pid, stat_fields.comm, &stat_fields.state,
&stat_fields.ppid, &stat_fields.pgrp, &stat_fields.session,
&stat_fields.tty_nr, &stat_fields.tpgid, &stat_fields.flags,
|
xgupta
changed the title
xgupta
requested review from
cyndyishida,
rupprecht,
keith,
aaupov,
maksfb,
rafaelauler,
ayermolo,
dcci,
ftynse,
makslevental,
stellaraccident,
grypp,
aartbik,
PeimingLiu,
yinying-lisa-li,
matthias-springer,
antiagainst,
kuhar,
nicolasvasilache,
Groverkss,
Superty,
dcaballe,
hanhanW,
banach-space and
MaheshRavishankar
as code owners
xgupta
requested review from
JDevlieghere
and removed request for
hanhanW,
maksfb,
rafaelauler,
Superty,
nicolasvasilache,
Endilll,
dcaballe,
cyndyishida,
PeimingLiu,
aartbik,
ayermolo,
tbaederr,
Groverkss and
yinying-lisa-li
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Please remove the formatting changes and limit this to just the specifier change. I know most of the time we'd want it formatted but for a fix as limited as this, the formatting change just gets in the way, and not formatting it doesn't block merging either. Otherwise this LGTM once that's done. |
|
The bot will complain again but we will ignore it :) |
There was a problem hiding this comment.
The field this is consuming is actually 17 bytes long, because the process name is in parenthesis. I suspect this will cause the function to reject any process whose name is longer than 13 characters.
The name field is actually quite hard to parse this way since it can contain any character (esp. parenthesis and spaces). Now, we could devise an algorithm to do that, but since the code is later opening /proc/$PID/status anyway, and status contains a superset of information, I think it'd be best to just delete this code and extract the information we want from there.
status parsing code also uses more modern and less error prone patterns.
The field it's overwriting is in a struct, so it has a lot of headroom for "safely" overflowing without hitting anything important. And since the the other fields are parsed after the string field, they probably just immediately overwrite the corrupted data. |
|
In that case, best not to store it at all at least until a better way to parse it is found - #100387. |
|
Thanks, @DavidSpickett for your work. |
The format specifier for the 'comm' field in the sscanf function was corrected to limit the input size and prevent potential buffer overflow. The '%s' specifier was replaced with '%15s' to ensure the 'comm' field does not exceed 15 characters.
Value 15 is chosen because 'comm' array have 16 length including ‘\0’ character at the end.
char comm[task_comm_len];
constexpr int task_comm_len = 16;
Caught by cppcheck -
lldb/source/Host/linux/Host.cpp:94:7: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
Fix #89710